Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build(deps): switch S3 HTTP client from urlconnection to apache #607

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Aug 14, 2024

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


See cryostatio/cryostat-helm#184

Description of the change:

Switches out the HTTP client library used by the S3 (storage) client from JDK urlconnection to Apache HTTP.

Motivation for the change:

https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/http-configuration.html

Apache HTTP should be more performant and robust than urlconnection, and supports the full S3 API feature set.

After splitting out the storage container to a separate Deployment with a separate Service I observe some connection flakiness and instability, where Cryostat API requests dealing with storage (archive actions, querying custom event templates) often respond with sporadic HTTP 500 responses. Retrying identical requests tends to work, so I wonder if switching out the backing HTTP client will help stabilize this.

How to manually test:

  1. Build PR, or use quay.io/andrewazores/cryostat:s3-apache-1
  2. With a Kubernetes or OpenShift cluster, deploy the image from this PR with split deployments a la feat(deployment): split out -db and -storage deployments cryostat-helm#184 : helm install cryostat --set authentication.openshift.enabled=true --set core.route.enabled=true --set core.image.repository=quay.io/andrewazores/cryostat --set core.image.tag=s3-apache-1 --set pvc.enabled=true ./charts/cryostat/
  3. Go to Event Templates view, upload a custom event template.
  4. Create a new recording (on Cryostat or another target) using the custom event template
  5. Wait some time, then repeatedly archive the recording
  6. Try to generate reports on archived recordings
  7. Delete archived recordings
  8. Delete the custom event template
  9. The above actions should all complete with a high degree of reliability, ie. few (ideally 0) HTTP 5xx exceptions

@andrewazores andrewazores added dependencies Pull requests that update a dependency file build safe-to-test labels Aug 14, 2024
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Aug 14, 2024
@andrewazores andrewazores removed the needs-triage Needs thorough attention from code reviewers label Aug 14, 2024
@andrewazores andrewazores marked this pull request as ready for review August 14, 2024 18:47
@andrewazores andrewazores requested a review from a team as a code owner August 14, 2024 18:47
@andrewazores andrewazores added chore Refactor, rename, cleanup, etc. and removed build labels Aug 14, 2024
@andrewazores
Copy link
Member Author

/build_test

Copy link

Workflow started at 8/14/2024, 3:09:34 PM. View Actions Run.

Copy link

No GraphQL schema changes detected.

Copy link

No OpenAPI schema changes detected.

Copy link

CI build and push: All tests pass ✅
https://github.com/cryostatio/cryostat/actions/runs/10393537211

@andrewazores andrewazores merged commit 8f86147 into cryostatio:main Aug 14, 2024
20 of 25 checks passed
@andrewazores andrewazores deleted the s3-client branch August 15, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactor, rename, cleanup, etc. dependencies Pull requests that update a dependency file safe-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants